adding “name” field to payload;updated nimble to version 7.3.4 #833
Conversation
|
I'd generally like to see unrelated changes split into different PRs. So in this case, I'd do one for the Nimble update and one for the For the |
In general, I'd agree, however in this case I feel like it adds unnecessary work since that change was required just to get tests to compile. Re the test, if we can add it quickly, we should. If not, which is what I suspect given our tests can't be run in isolation, maybe skip it for now since this is overall a one-line change. |
There was a problem hiding this comment.
In general, I'd agree, however in this case I feel like it adds unnecessary work since that change was required just to get tests to compile.
I think that's good reason to separate these out, as it sounds like the dependency update change has value independently of the device model portion of the change.
For instance we might find a bug in how we're collecting the device model, and might want to revert only that portion of the change. Or we might find a bug in the dependency change, and might want to revert only that portion of the change. Breaking them into two PRs would allow us to revert them independently.
This isn't a strict requirement though, so happy to approve!
What does this PR do?
Add's collection of "name" field to the iOS library, and updates Nimble to version 7.3.4
Where should the reviewer start?
N/A
How should this be manually tested?
N/A
Any background context you want to provide?
Done to match collection of data in Android library. Android was collecting both the model and name of the device, whereas iOS was only collecting the model. Libraries will now have parity and match the Segment Spec.
What are the relevant tickets?
https://segment.atlassian.net/browse/LIB-837
Screenshots or screencasts (if UI/UX change)
N/A
Questions:
No, this is how functionality was outlined in the Spec to begin with.
N/A
Unsure